fix(noema): fail closed at the credential egress boundary - #1279
fix(noema): fail closed at the credential egress boundary#1279seonghobae wants to merge 5 commits into
Conversation
|
Warning Review limit reachedNext included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
b19c5b452cf53a5b5a85d9805efaa1899cf0a04b. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Strix Security Scan/strix: FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/32661188447/job/97250595093)
- Strix Security Scan/strix: failure (https://github.com/ContextualWisdomLab/.github/actions/runs/32661188447/job/97250595093)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: noema-credential-egress-boundary.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: noema-credential-egress-boundary.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: noema_review_gate.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: noema_review_gate.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test (3 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (3 files)"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed. Findings1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
Failed checks:
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: noema-credential-egress-boundary.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: noema-credential-egress-boundary.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: noema_review_gate.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: noema_review_gate.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test (3 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (3 files)"]
R4 --> V4["targeted test run"]
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact-head formal review request for 92c8b95. Inspect the complete current diff, especially Noema credential egress boundary, fail-closed behavior, and secret handling. Publish a substantive Reviews API verdict tied only to this exact SHA, with changed-file walkthrough, security/control-plane impact, test gaps, and residual uncertainty. Do not reuse predecessor evidence. @opencode-agent |
|
Exact-head current identity is |
92c8b95 to
2adc8c4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact-head validation after fixing the Devin IPv6 finding:
The remaining TCP_NODELAY note is informational and does not affect credential routing or correctness; it is not being expanded into this bounded fix. Hosted checks and a fresh independent review are still pending, so this PR is not being merged or force-merged. |
There was a problem hiding this comment.
Pull request overview
OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
721a36f24616343029a291f02db32610f470a884. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Strix Security Scan/strix: FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/32723348685/job/97419259259)
- Strix Security Scan/strix: failure (https://github.com/ContextualWisdomLab/.github/actions/runs/32723348685/job/97419259259)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: noema-credential-egress-boundary.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: noema-credential-egress-boundary.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: noema_review_gate.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: noema_review_gate.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test (3 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (3 files)"]
R4 --> V4["targeted test run"]
…ain-replacement-20260824 # Conflicts: # CHANGELOG.md # scripts/ci/noema_review_gate.py
Resolved the stale merge conflict against
|
| class PinnedHTTPHandler(urllib.request.HTTPHandler): | ||
| """urllib handler that uses validated numeric destinations for HTTP requests.""" | ||
|
|
||
| def __init__(self, addresses: frozenset[IpAddress]) -> None: | ||
| """Bind this handler to one prevalidated DNS result set.""" | ||
| super().__init__() | ||
| self._addresses = addresses | ||
|
|
||
| def http_open(self, req: urllib.request.Request) -> Any: | ||
| """Open an HTTP request without resolving its hostname again.""" | ||
| return self.do_open( | ||
| lambda host, **kwargs: PinnedHTTPConnection( | ||
| host, validated_addresses=self._addresses, **kwargs | ||
| ), | ||
| req, | ||
| ) | ||
|
|
||
|
|
||
| class PinnedHTTPSHandler(urllib.request.HTTPSHandler): | ||
| """urllib handler that pins TCP while preserving HTTPS hostname verification.""" | ||
|
|
||
| def __init__(self, addresses: frozenset[IpAddress]) -> None: | ||
| """Bind this handler to one prevalidated DNS result set.""" | ||
| super().__init__() | ||
| self._addresses = addresses | ||
|
|
||
| def https_open(self, req: urllib.request.Request) -> Any: | ||
| """Open HTTPS using the validated address set and original URL hostname.""" | ||
| return self.do_open( | ||
| lambda host, **kwargs: PinnedHTTPSConnection( | ||
| host, validated_addresses=self._addresses, **kwargs | ||
| ), | ||
| req, | ||
| context=self._context, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Validated transport remains internally consistent
Custom handlers replace urllib defaults and pin sockets to validated addresses. TLS still verifies the original hostname, while the exact loopback sidecar remains reachable.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _socket_target(address: IpAddress, port: int) -> tuple[str, int]: | ||
| """Return a socket destination that contains only a validated IP literal.""" | ||
| return (str(address), port) |
There was a problem hiding this comment.
📝 Info: IPv6 destination shape is valid
socket.create_connection accepts a two-element host-and-port pair for IPv6 literals. The four-element form applies to lower-level socket addresses, not this API.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if resolve_endpoint_addresses(hostname, port) != addresses: | ||
| raise ValueError("Noema endpoint DNS addresses changed during the request") |
There was a problem hiding this comment.
📝 Info: DNS recheck cannot redirect the request
The post-response lookup only validates identity stability. The completed request already used the prevalidated numeric set, so later DNS changes cannot redirect it.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Outcome
Replaces the conflicted and scope-polluted #930 with a clean current-main change.
This PR does not import contextual-orchestrator, alter provider routing, change model selection, or modify model parameters.
Evidence
Supersedes #930. Preserves the trusted loopback consumer contract from #1120.